Skip to content

Conversation

@metaskills
Copy link
Member

This branch will be squashed and merged when complete. For now, we just got the adapter tests passing. I have yet to run the full ActiveRecord suite to see what else needs to be done but I suspect it will take a few more weeks till we are ready for a pre-release and testing.

screen shot 2016-08-10 at 8 02 29 pm

SQLSERVER_STATEMENT_PREFIX = 'EXEC sp_executesql '
SQLSERVER_PARAM_MATCHER = /@\d+ =/
SQLSERVER_STATEMENT_PREFIX = 'EXEC sp_executesql '.freeze
SQLSERVER_PARAM_MATCHER = /@\d+ = (.*)/.freeze
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regex literals are always frozen in Ruby.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks... keep it coming :)

@sgrif
Copy link

sgrif commented Aug 11, 2016

#RandomUnsolicedCodeReview

def view_exists?(table_name)
identifier = SQLServer::Utils.extract_identifiers(table_name)
views.include? identifier.object
end
Copy link

@BrandonRoehl BrandonRoehl Aug 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the identifier need to be stored in memory?
From

identifier = SQLServer::Utils.extract_identifiers(table_name)
views.include? identifier.object

to

views.include? SQLServer::Utils.extract_identifiers(table_name).object

For a one liner that doesn't stamp out memory. Since it is only used once.

@metaskills metaskills merged commit 67d1e62 into master Aug 20, 2016
@BrandonRoehl
Copy link

Yay merge!

@metaskills
Copy link
Member Author

NOTICE

Please keep in mind this is only the core adapter tests passing. I'll be making individual PRs as work progresses for other things. There is still much work to do.

@metaskills metaskills deleted the rails5 branch December 9, 2016 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants